Skip to content

fs: accept all valid utf8 values in fast paths - #64341

Open
hamidrezaghavami wants to merge 1 commit into
nodejs:mainfrom
hamidrezaghavami:fix-fs-utf8-fastpath
Open

fs: accept all valid utf8 values in fast paths#64341
hamidrezaghavami wants to merge 1 commit into
nodejs:mainfrom
hamidrezaghavami:fix-fs-utf8-fastpath

Conversation

@hamidrezaghavami

@hamidrezaghavami hamidrezaghavami commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Fixes: #49888

This PR introduces a lightweight isUtf8Encoding helper to ensure fs.readFileSync, fs.writeFileSync, and encodeRealpathResult use the fast C++ path for all valid variations of the utf8 encoding string (utf8, utf-8, UTF8, UTF-8).

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jul 7, 2026
@hamidrezaghavami
hamidrezaghavami force-pushed the fix-fs-utf8-fastpath branch 2 times, most recently from bfaade2 to 1d3f916 Compare July 7, 2026 17:40
@codecov

codecov Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.36%. Comparing base (1f26576) to head (242013b).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #64341      +/-   ##
==========================================
+ Coverage   90.35%   90.36%   +0.01%     
==========================================
  Files         792      792              
  Lines      275500   275506       +6     
  Branches    52791    52797       +6     
==========================================
+ Hits       248932   248971      +39     
+ Misses      16992    16945      -47     
- Partials     9576     9590      +14
Files with missing lines Coverage Δ
lib/fs.js 97.33% <100.00%> (+<0.01%) ⬆️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@anonrig

anonrig commented Jul 7, 2026

Copy link
Copy Markdown
Member

I don't think we should land this. The more checks we have, the more performance impact it would create. Do you have any examples out there that is impacted by this? Does people really pass UTF8? If not, let's close this.

@hamidrezaghavami

hamidrezaghavami commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Hi @anonrig thanks for the review!

I completely understand the concern regarding the performance impact on the fast path. I picked this up to resolve issue #49888, as there seemed to be some inconsistency in how variations like utf-8 (which is common in HTTP headers/HTML meta tags) bypass the C++ fast path and fall back to the slower route.

That said, if the consensus is that the branch/check overhead for the 99% of users passing standard utf8 outweighs the benefit for the edge cases, I am perfectly happy to close this PR. Let me know what you think is best for the module!

@TheOneTheOnlyJJ TheOneTheOnlyJJ mentioned this pull request Jul 10, 2026
Open
4 of 5 tasks
@hamidrezaghavami

hamidrezaghavami commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

The commit message linting and sign-off trailer are fixed, and all CI checks are passing! Could someone from the @nodejs/fs team please take a look when you have a moment? Thanks!

@trivikr
trivikr requested a review from anonrig July 21, 2026 05:31
anonrig added a commit to anonrig/node that referenced this pull request Aug 16, 2026
@anonrig
fs: accept all utf8 encoding aliases on buffer I/O fast paths
f1bd58e 
Use normalizeEncoding so readFileSync/writeFileSync take the C++
fast path for UTF8, UTF-8, and mixed-case variants, not only
lowercase utf8/utf-8.

Refs: nodejs#64341
@trivikr trivikr added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Aug 23, 2026
@trivikr

trivikr commented Aug 23, 2026

Copy link
Copy Markdown
Member

It looks like this commit is cherry-picked in #64396

@hamidrezaghavami

hamidrezaghavami commented Aug 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It looks like this commit is cherry-picked in #64396

Yes, @anonrig integrated my changes into his larger PR (#64396) to avoid merge conflicts. I think this one can be closed as superseded once his branch is ready to land!

@hamidrezaghavami

hamidrezaghavami commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Hey @trivikr! Since #64396 is currently blocked by conflicts and Windows CI failures, would it make sense to go ahead and merge this one first, since it already has approvals and green tests? That way, this UTF-8 logic is safely secured in main, and the other PR can just naturally rebase over it when it is ready. Let me know what you think!

@trivikr

trivikr commented Aug 26, 2026

Copy link
Copy Markdown
Member

I think this should land. WDYT @anonrig?

@hamidrezaghavami

hamidrezaghavami commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@trivikr Friendly ping on this! Since you mentioned this should land, and all 30 checks are green, could we move forward with merging it, or should we tag someone else since anonrig seems busy? I defer to your judgment as a core maintainer on the next steps.

@hamidrezaghavami

hamidrezaghavami commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Hi @jasnell @gurgunday — gentle ping on this when you have a moment. Changes are approved and ready for CI. Thanks!

@hamidrezaghavami
hamidrezaghavami force-pushed the fix-fs-utf8-fastpath branch 4 times, most recently from 55bbc5a to 221cbd5 Compare September 29, 2026 13:48
@hamidrezaghavami
fs: accept all valid utf8 values in fast paths
242013b 
Signed-off-by: Hamid Reza Ghavami <hamidr.ghavami@gmail.com>
@hamidrezaghavami

hamidrezaghavami commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

@jasnell @gurgunday The merge conflicts are fully resolved, and the JS is clean.
It looks like the "lint-sh" and "test-macOS" GitHub Actions are flaking (macOS failed after just 1 minute, and the shell linter shouldn't be impacted by these "lib/fs.js" changes).

Could you take a look at re-approving and triggering a fresh Jenkins CI run when you have a moment? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@gurgunday gurgunday gurgunday approved these changes
@anonrig anonrig Awaiting requested review from anonrig

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fs: utf8 fast paths don't accept all valid utf8 values

6 participants

@hamidrezaghavami @anonrig @trivikr @jasnell @gurgunday @nodejs-github-bot